Remote property accessor on branch branch should not throw an exception #823

jamill · 2014-09-11T15:38:54Z

When a branch is under the refs/remotes/ namespace, but a specific remote cannot be resolved,
then the Remote property accessor should return null instead of throwing an exception.

jamill · 2014-09-11T15:43:27Z

LibGit2Sharp.Tests/BranchFixture.cs

This one is a bit funny... it returns GitError.Ambiguous instead of GitError.NotFound, but I don't think it is actually ambiguous. Neither 53fc32d or 0266163 can be resolved to a commit, but different error codes are returned for these two cases (53fc32d resolves to a tree, 0266163 resolves to a blob).

Ambiguous is indeed an odd error to return. I would expect this to return some form of invalid, since you can't make a branch point to a blob.

This error is coming when LibGit2Sharp attempts to peel the object (via git_object_peel(...)) to a commit. git_object_peel is returning different error codes.

I agree that LibGit2Sharp also has room to be more helpful. Currently, it is just saying that it could not find a commit for the given spec (which, is not wrong). It could be improved to be more specific and indicate that the given spec refers to an object of an invalid type.

Yeah, that's what I would expect from such an API. "not found" leads me to believe that I gave it something which doesn't exist, when the case is that I gave it something which exists, but is of the wrong type.

Regarding repo.CreateBranch(name, "refs/tags/point_to_blob") and repo.CreateBranch(name, "53fc32d"), how about throwing InvalidSpecificationException instead by giving a more detailed message (Something like "Provided target cannot be resolved to a commit")?

jamill · 2014-09-12T13:25:47Z

@nulltoken - I tweaked this PR so that git_branch_remote now takes an optional argument to indicate if it should throw on a not found error. This function can also return GIT_EAMBIGUOUS as well - which also should be handled for the property accessor case. I will think about this some more.

jamill · 2014-09-19T14:57:07Z

@nulltoken - I was looking at why git_object_peel returns different errors depending on whether it is attempting to peel a Tree or a Blob. In commit libgit2/libgit2@bc05f30c we can see that returning different error codes is pretty obviously the intent of the code, but I am not sure why this is the desired behavior? Do you know why it is this way (you are listed as the author of this commit 😃 ).

jamill · 2014-09-19T15:41:57Z

how about throwing InvalidSpecificationException instead by giving a more detailed message (Something like "Provided target cannot be resolved to a commit")?

InvalidSpecificationException - This is The exception that is thrown when the provided specification is syntactically incorrect., which wouldn't fit this case. So, we can:

Throw a NotFoundException to indicate that we could not find a commit for the spec (when the spec is syntactically well formed). I think this covers the basic scenario and is not an incorrect statement (it could not find the commit for the given spec). Later, we
Throw a different / new exception type to indicate that the spec can be resolved to an object, but the object is not of the desired type (CannotDerefenceToACommitException, CannotResolveToCommitException, or maybe CannotResolveToTypeException).

If we think there is value in the caller knowing they passed in a spec that matches some object, but not the expected type (and I think there is), then option 2 seems like the way to go. The downside would be yet another exception that callers of this method would have to know about and handle.

The main problem I was trying to solve with this PR is to not have the property accessor throw an exception, so I feel like this is a bit of feature creep - but I can try and see how much work this turns out to be.

jamill · 2014-09-24T15:37:41Z

@nulltoken OK - the latest proposal is to move entry point for getting the remote of a remote-tracking branch into it's own function. My thinking is that this is a more complicated operation with greater potential for failure and so it deserves it's own function. This restores the Remote property accessor to just return the remote that a local branch will push and pull from.

Thoughts?

nulltoken · 2014-09-24T19:44:05Z

I'm not sure exposing a property and a method that both seems to perform the same thing would be very user friendly.

How about starting by

Making sure the property doesn't throw
Maybe Log as a warning the cause of the issue when there is one (cf. Introduce tracing #832 /cc @ethomson)

If that doesn't turn to be good enough from a user perspective, we may eventually promote this property into a method and make it throw explicit exceptions.

nulltoken · 2014-09-24T20:04:42Z

LibGit2Sharp.Tests/LibGit2Sharp.Tests.csproj

You've just been VS'ed 😉

The Branch.Remote property can throw when it attempts to resolve a remote for a remote-tracking branch, and the remote cannot be found or is ambiguous. In those situations, Branch.Remote should return null. If there is a need for the specific exception to be returned, we can look at transitioning to a method for this functionality.

nulltoken · 2014-09-25T05:58:55Z

👍 for me.

nulltoken · 2014-09-26T19:22:24Z

🚀

jamill reviewed Sep 11, 2014
View reviewed changes

jamill force-pushed the jamill/remote_error branch from ea8f740 to 0b7bd7f Compare September 12, 2014 12:47

jamill force-pushed the jamill/remote_error branch from bbbdc00 to e16864e Compare September 12, 2014 13:54

jamill force-pushed the jamill/remote_error branch 2 times, most recently from 35dc1a4 to fcbe836 Compare September 24, 2014 15:34

jamill force-pushed the jamill/remote_error branch from fcbe836 to d4b2fb7 Compare September 24, 2014 15:40

jamill force-pushed the jamill/remote_error branch from d4b2fb7 to 49d24be Compare September 24, 2014 19:58

nulltoken reviewed Sep 24, 2014
View reviewed changes

LibGit2Sharp.Tests/LibGit2Sharp.Tests.csproj Outdated

Copy link

Member

nulltoken Sep 24, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've just been VS'ed 😉

jamill force-pushed the jamill/remote_error branch from 49d24be to 85be650 Compare September 24, 2014 20:58

jamill added 2 commits September 24, 2014 17:20

Verify that resolving a remote-tracking branch's remote can throw

0e5bfbc

jamill force-pushed the jamill/remote_error branch from 85be650 to 0e5bfbc Compare September 24, 2014 21:22

nulltoken added this to the v0.20 milestone Sep 26, 2014

nulltoken merged commit 0e5bfbc into vNext Sep 26, 2014

nulltoken deleted the jamill/remote_error branch September 26, 2014 19:22

Remote property accessor on branch branch should not throw an exception #823

Remote property accessor on branch branch should not throw an exception #823

Uh oh!

Conversation

jamill commented Sep 11, 2014

Uh oh!

jamill Sep 11, 2014

Choose a reason for hiding this comment

Uh oh!

carlosmn Sep 11, 2014

Choose a reason for hiding this comment

Uh oh!

jamill Sep 11, 2014

Choose a reason for hiding this comment

Uh oh!

carlosmn Sep 11, 2014

Choose a reason for hiding this comment

Uh oh!

nulltoken Sep 11, 2014

Choose a reason for hiding this comment

Uh oh!

jamill commented Sep 12, 2014

Uh oh!

jamill commented Sep 19, 2014

Uh oh!

jamill commented Sep 19, 2014

Uh oh!

jamill commented Sep 24, 2014

Uh oh!

nulltoken commented Sep 24, 2014

Uh oh!

nulltoken Sep 24, 2014

Choose a reason for hiding this comment

Uh oh!

nulltoken commented Sep 25, 2014

Uh oh!

nulltoken commented Sep 26, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants